-
Notifications
You must be signed in to change notification settings - Fork 17.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fast Copter Fences #25698
Fast Copter Fences #25698
Conversation
I am testing using this script: https://gist.github.com/andyp1per/85e5e29dac031e4c93d6e83d8f71641f Here is an example log: 00000056.zip The copter flies through the fence at a very high speed and is about 50m past it before the fence kicks in. On the way down the fence just about kicks in before it hits the ground. |
996dae8
to
d427c54
Compare
5cee51e
to
35db0a8
Compare
35db0a8
to
340634a
Compare
does this fix plane also? I think mine fixes both....note also that autoenable handles floor differently for each case |
Well it fixes the bugs I want fixed of which there are a few. Copter is actually missing the autoenable feature which this implements amongst other things. What is the bug you think that needs fixing in plane? |
5d55085
to
c6421f5
Compare
Fences have the following issues:
Any changes to AC_Fence module without considering how it may impact Plane or future fixes for plane fence issues is a problem IMHO Having floor always enable once MIN_ALT is reached by itself wont impact future fixes and only impacts the MIN_ALT fence, whether that fence is setup or not...but it must be able to do this after disables from any altitude, and after landing for both Copter and Plane And CI heavily incorporates all the existing wierdness of fences, so virtually any ,even minor, changes breaks something! |
ArduCopter/fence.cpp
Outdated
@@ -24,7 +24,8 @@ void Copter::fence_check() | |||
if (new_breaches) { | |||
|
|||
if (!copter.ap.land_complete) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a bit odd that we don't print if landed. Its the current behavior of course. But now it will print when the breach clears in all caces, so you can end up reporting the clear but not the breach.
libraries/AC_Fence/AC_Fence.cpp
Outdated
// @Description: 0:Disable mode change following fence action until fence breach is cleared. When bit 1 is set the allowable flight areas is the union of all polygon and circle fence areas instead of the intersection, which means a fence breach occurs only if you are outside all of the fence areas. | ||
// @Bitmask: 0:Disable mode change following fence action until fence breach is cleared, 1:Allow union of inclusion areas | ||
// @Description: 0:Disable mode change following fence action until fence breach is cleared. When bit 1 is set the allowable flight areas is the union of all polygon and circle fence areas instead of the intersection, which means a fence breach occurs only if you are outside all of the fence areas. When bit 2 is set any active minimum altitude floor will be disabled. | ||
// @Bitmask: 0:Disable mode change following fence action until fence breach is cleared, 1:Allow union of inclusion areas, 2:Auto-disable floor fence on disarming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you have this bit ever disabled? I would have thought you would also want the floor disabled when disarming if you have AutoEnable::ALWAYS_ENABLED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IamPete1 because the tests check for the current disarmed behaviour. We can change the tests of course, but we need to be all agreed that this is the correct behaviour - because it is a change. At the moment I have added new tests for the new behaviour.
43c5644
to
1a0c9ad
Compare
I think this is fixed by this PR, but its possible there is something special going on in plane.
Yes, this is the main fix of this PR. Without this MIN_ALT is mostly useless on Copter.
Can you expand on this? Current behaviour is that fence remains enable but is ignored for landing so that you cannot rearm. This PR provides an option which automatically clears the breach once you have landed so that you can rearm without rebooting.
For copter see above - I think this is partially fixed (second part). I'm not sure that the first part is the wrong behavior - once your breach action has engaged you want it to stay engaged. I did fix a bug which preventing scripting seeing new breaches.
I don't quite get this one but I think its plane specific as auto enable is not currently supported on copter without this PR. With this PR I am able to land ok (at least in RTL). |
@andyp1per any change to AC_Fences must be THOROUGHLY tested for impacts on Plane....it took me two days of SITL sims on just MIN_ALT fence to find all the hidden anomalies... |
Sure. The solution here, of course, is making sure that the autotests cover the edge cases. I have added some tests but more can be added - be good to know what tests you think are missing. I also have spent a long time flying this in RF, that also enabled me to uncover many of these hidden "gems". Clearly the fencing code is not as widely used as we suspect otherwise they would have been revealed sooner. |
@@ -147,6 +147,9 @@ const AP_Scheduler::Task Copter::scheduler_tasks[] = { | |||
|
|||
SCHED_TASK(rc_loop, 250, 130, 3), | |||
SCHED_TASK(throttle_loop, 50, 75, 6), | |||
#if AP_FENCE_ENABLED | |||
SCHED_TASK(fence_check, 25, 100, 7), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth moving fence checking to an IO callback, or add a FENCE_OPTIONS bit for fast fence checking
We discussed at length on the dev call but disagreed about how fast the fence check could be run. Various suggestions came up re adding a compile time update rate parameter, a parameter option, dependent upon the CPU time etc. I think it would be good to first get a baseline of how expensive the fence check is. For example, how complex can the fence be (e.g. how many polygon points) and still complete within 1ms on an F4 CPU? |
1a0c9ad
to
99f020e
Compare
Fence fixes split out into #25875 |
99f020e
to
bd98e13
Compare
On a CubeBlack I get about 10us per cycle for general fence checks, if I add a poly fence with 25 points It goes to about 22us percycle, if I increase this to include an exclusion fence with another 42 points I get about 33us. So being ultra conservative lets say we can get 1 point per micro that means we can have a 1000 point fence for 1ms on an F4, but in all likelihood considerably more than this. Not sure what this means for my update rate, but I think we can go a decent amount faster than 3Hz. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the testing. Personally I'd make it 10hz because of the law of diminishing returns but it's probably fine at 25hz too. A very fast copter flying at 20m/s will get 6.6m breach currently, only 2m at 10hz, 0.8m at 25hz.
bd98e13
to
04e2877
Compare
Maybe for 4.5.1 if the other fence PR gets finally approved |
An attempt at checking fences quickly to avoid massive breaches
Adds support for auto-enabling fence floors on copter
Current master is fixed at 3Hz so if you are going 30m/s you will be 10m before the fence before it is even checked
Demo: https://youtu.be/8umjR5EAKfg